-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: only enable full scan when enable_full_scan is set explicitly forleast request lb #30794
Conversation
…rleast request lb Signed-off-by: wbpcode <[email protected]>
cc @lizan could you take a look as a senior maintainer when you have free time? Thanks. |
cc @barroca |
@barroca part of your change had to be reverted due to a regression. I think this can be fixed by, during the full scan, picking a random host from those that have the lowest rq count, instead of the first host with that count. Would you be willing to make that fix? |
…citly forleast request lb (envoyproxy#30794)" This reverts commit e93e556. Revert "Fix least request lb not fair (envoyproxy#29873)" This reverts commit 3ea2bc4. restore api Signed-off-by: Kuat Yessenov <[email protected]> fix merge Signed-off-by: Kuat Yessenov <[email protected]>
To be explicit here: if active_rq is equal across all hosts, and the host list is in the same order for all callers, you can end up picking the same host in the list every time if active_rq is the same for all of them. This can happen if you have a Envoy cluster that is rarely called but responds with low latency, so active_rq tends to be zero across all hosts. Maybe we start the for loop from a random index when doing the full scan, and that should alleviate this issue as well? |
Commit Message: fix: only enable full scan when enable_full_scan is set explicitly forleast request lb
Additional Description:
#29873 introduced a minor breaking change. The full scan will be enabled automatically when host num is less than choice count.
Although from technical point, I think this change basically harmless and could make a better load balancing result, a new runtime guard should be enough.
But in practice, it effects our downstream users in a unexpected way. See #30768 (comment)
No change log is updated or added because our change log or doc doesn't mention that full scan will be enabled automatically when host num less than choice count.
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]